I believe this is low risk and can be backported as a bugfix (there are technically two "bugs": the handle never being unlocked / closed in certain cases, and all the issues caused by the missing opcache invalidation)

What bugs are you trying to fix? Are those bugs that you encountered yourself?

I’d expect PHP to release all locks on crash, I’m surprised the finally block is needed. Otherwise we’d need this kind of thing in a lot of places.
Also you are silencing the fopen errors, which will make understanding the situation harder in cases where fopen fails.

What bugs are you trying to fix? Are those bugs that you encountered yourself?

Opcache. For example pulsejet/memories#578 (comment)
I've lost count how many times something similar has been reported

I'm aware the server tuning docs mention the pitfalls of disabling revalidation, but revalidating one file (config.php) and every file on every request aren't the same thing (not even close). By revalidating just the config file for every request (which carries almost no cost), disabling general revalidation becomes much more realistic.

I’d expect PHP to release all locks on crash, I’m surprised the finally block is needed. Otherwise we’d need this kind of thing in a lot of places.

Correct, it's not strictly needed but this is cleaner / more resilient. In case the exception is caught, we no longer rely on PHP GC behavior the request to end for the file to unlock.

Also you are silencing the fopen errors, which will make understanding the situation harder in cases where fopen fails.

Errors like? There were checks on existence and readability so this would've never logged anything either way (this one is a performance fix to save a couple of kernel calls)

Also note, the locked/critical block should always be as small as possible (generally speaking), and the shortest way to do it is this.

What bugs are you trying to fix? Are those bugs that you encountered yourself?

Opcache. For example pulsejet/memories#578 (comment) I've lost count how many times something similar has been reported

I'm aware the server tuning docs mention the pitfalls of disabling revalidation, but revalidating one file (config.php) and every file on every request aren't the same thing (not even close). By revalidating just the config file for every request (which carries almost no cost), disabling general revalidation becomes much more realistic.

We should not impact everyone to handle the case of a badly configured opcache. If opcache is used without revalidation it must be manually cleaned on app install/update, no? Or maybe it can be done automatically, don’t know. But disabling opcache for everyone on config file looks overkill.

I’d expect PHP to release all locks on crash, I’m surprised the finally block is needed. Otherwise we’d need this kind of thing in a lot of places.

Correct, it's not strictly needed but this is cleaner / more resilient. In case the exception is caught, we no longer rely on PHP GC behavior the request to end for the file to unlock.

Good point, did not think of the case it’s catched.

Also you are silencing the fopen errors, which will make understanding the situation harder in cases where fopen fails.

Errors like? There were checks on existence and readability so this would've never logged anything either way (this one is a performance fix to save a couple of kernel calls)

Any fopen failure other than non-existing file.
https://github.com/php/php-src/blob/master/main/fopen_wrappers.c#L700 too long path for instance.

We should not impact everyone to handle the case of a badly configured opcache. If opcache is used without revalidation it must be manually cleaned on app install/update, no? Or maybe it can be done automatically, don’t know.

I wouldn't say it's badly configured, this is actually good for performance (even the server tuning doc makes a passing mention). The problem isn't with app installation; opcache is reset when upgrading anyway. But the config file changing is what is the problem; this can happen more often and give rise to very subtle bugs.

But disabling opcache for everyone on config file looks overkill.

Opcache is not disabled, even on the config file. Just the timestamp is revalidated on every request, which is very minimal overhead (notice $force=false in the invalidate). Notably, checking the timestamp is actually much faster than opening and locking the file; things that we already do anyway. While the cost of this is negligible, the correctness guarantee it provides is pretty big.

Here's another way of looking at it: if we consider turning off revalidation as "bad", then the total cost paid here is basically nothing since the timestamp was validated anyway. On the other hand, just validating the config file makes turning off revalidation generally a "valid" opcache configuration (which we actually expect to work well), and reduce the overall overhead.

Any fopen failure other than non-existing file.

Point taken, though I wouldn't consider this a big deal. The check for a valid config file is needed only once (ever), but we pay the cost for every single request, which seems very excessive.

Anyway, can revert this one if you prefer.

More thoughts?

Can you detail the rationale on "Skip even for non-main fails when we can't open it (it'll crash later anyway otherwise)" ? With this PR the admin will have a hard time noticing the permissions on an additional config file are wrong, nothing is logged or shown.

True ... the current behavior (before this patch) is odd; it just goes on with a false in the file and throws a TypeError. Looking at the error you can't get any idea what is wrong either.

I've updated the patch to show a better error. (note we can't actually throw here since the exception gets caught and the config not being loaded leads to more issues)

Before	After

Also updated PR description

I can't merge due to CI failure, seems unrelated

Rerunning CI

/backport to stable28

/backport to stable29

The backport to stable29 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable29
git pull origin stable29

# Create the new backport branch
git checkout -b backport/44230/stable29

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 15855876

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/44230/stable29

Error: No changes found in backport branch

Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

/backport to stable29

@szaimen this is already merged to 29

Ah I see